Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opt: fix histogram filtering with infinite upper bound #100763

Merged
merged 1 commit into from
Apr 10, 2023

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Apr 5, 2023

This commit fixes a bug that caused the filtering a histogram bucket
with an infinite upper bound to produce a bucket with a numRange of
NaN. This led to bad query plans because the NaN did not reduce the
input expression's row count at all.

Fixes #100582

Release note (bug fix): A bug has been fixed that caused suboptimal
query plans when a table contained infinite values, like
'+Infinity'::DECIMAL. This bug is present since versions v22.1, and
likely earlier. It could also be triggered in rare cases when table
statistics forecasts created a forecasted bucket with an infinite value.

@mgartner mgartner requested review from rytaft and michae2 April 5, 2023 21:44
@mgartner mgartner requested a review from a team as a code owner April 5, 2023 21:44
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Thank you! Nice fix!

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)


pkg/sql/opt/props/histogram.go line 754 at r1 (raw file):

		// to the new bucket.
		numRange = b.NumRange * rangeAfter / rangeBefore
		if !math.IsNaN(numRange) {

Does this happen because rangeAfter is NaN? Or Infinity?

@mgartner
Copy link
Collaborator Author

mgartner commented Apr 6, 2023

pkg/sql/opt/props/histogram.go line 754 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Does this happen because rangeAfter is NaN? Or Infinity?

It happens when both rangeAfter and rangeBefore are infinity. https://go.dev/play/p/e5G1HW3ZaRr

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @michae2)

@mgartner mgartner added backport-22.2.x backport-23.1.x Flags PRs that need to be backported to 23.1 labels Apr 10, 2023
This commit fixes a bug that caused the filtering a histogram bucket
with an infinite upper bound to produce a bucket with a `numRange` of
`NaN`. This led to bad query plans because the `NaN` did not reduce the
input expression's row count at all.

Fixes cockroachdb#100582

Release note (bug fix): A bug has been fixed that caused suboptimal
query plans when a table contained infinite values, like
`'+Infinity'::DECIMAL`. This bug is present since versions v22.1, and
likely earlier. It could also be triggered in rare cases when table
statistics forecasts created a forecasted bucket with an infinite value.
@mgartner mgartner force-pushed the 100582-fix-nan-histogram branch from a3b79b5 to 7b8ad4c Compare April 10, 2023 13:17
@mgartner
Copy link
Collaborator Author

TFTRs!

bors r+

@craig craig bot merged commit 82e09ed into cockroachdb:master Apr 10, 2023
@craig
Copy link
Contributor

craig bot commented Apr 10, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opt: histogram buckets with +Infinity upper bounds result in bad query plans
4 participants